Add configurable truncation for string columns#19146
Add configurable truncation for string columns#19146jaykanakiya wants to merge 6 commits intoapache:masterfrom
Conversation
| @JsonProperty("createBitmapIndex") Boolean createBitmapIndex, | ||
| @JsonProperty("maxStringLength") @Nullable Integer maxStringLength |
There was a problem hiding this comment.
drive by comment (i'll have a closer look at rest of PR later)
instead of adding additional arguments here, I was hoping to deprecate these arguments in favor of adding a column format spec similar to was done for auto/json columns in #17762, which could serve as a reference for how this should be wired up. I was planning to move the existing createBitmapIndex and multiValueHandling into such a spec, but just haven't got to it yet. I think this would be much cleaner and less disruptive to call sites going forward. It also allows wiring up to IndexSpec to be able to define job level defaults as a middle place between per column and system wide.
There was a problem hiding this comment.
Thanks for taking a look @clintropolis. Adding something like StringCommonFormatColumnFormatSpec would make it cleaner and makes sense to consolidate the configs there. Since it seems like a bigger refactor, does it make sense to do it in a follow up? Let me know what you think.
| */ | ||
| private static DimensionSchema.MultiValueHandling STRING_MV_MODE = DimensionSchema.MultiValueHandling.SORTED_ARRAY; | ||
| private static IndexSpec DEFAULT_INDEX_SPEC = IndexSpec.builder().build(); | ||
| private static int MAX_STRING_LENGTH = 0; |
There was a problem hiding this comment.
Would it make sense to set this to Integer max value? In case this is used elsewhere in the future there wouldn't need to explicit handling for 0 like you have in truncateIfNeeded
There was a problem hiding this comment.
We are using NON_DEFAULT to not serialize the default value and I think for integer jackson's default is 0. If we set the MAX_STRING_LENGTH default as int max, it'll serialize this value for each dimension.
| public SideEffectRegisterer initDimensionHandlerAndMvHandlingMode(DefaultColumnFormatConfig formatsConfig) | ||
| { | ||
| setStringMultiValueHandlingModeIfConfigured(formatsConfig.getStringMultiValueHandlingMode()); | ||
| setMaxStringLengthIfConfigured(formatsConfig.getMaxStringLength()); |
There was a problem hiding this comment.
You can take a look at druid.indexing.formats.stringMultiValueHandlingMode in BuiltInTypesModuleTest It would be good to have some test coverage for the new property
There was a problem hiding this comment.
Added tests for this property.
| this.stringMultiValueHandlingMode = validateMultiValueHandlingMode(stringMultiValueHandlingMode); | ||
| this.nestedColumnFormatVersion = nestedColumnFormatVersion; | ||
| this.indexSpec = indexSpec; | ||
| this.maxStringLength = maxStringLength; |
There was a problem hiding this comment.
Here we should validate that configured maxStringLength > 0, otherwise we can throw an exception or log that we are falling back to the default value
There was a problem hiding this comment.
Added validations.
| ); | ||
| } | ||
|
|
||
| private long verifyEncodedValues( |
There was a problem hiding this comment.
Leaving a comment for us to revisit later:
I'm curious what the truncation behavior would be like for MVD
There was a problem hiding this comment.
I did have a chat with @abhishekrb19 about multi value strings and we decided to leave out truncation for those.
There was a problem hiding this comment.
Ahh okay. I see we're truncating for MVD's with a single value. Would be good to add a test case for that also if that's the intended behavior
|
|
||
| @AfterEach | ||
| public void beforeEach() | ||
| @After |
There was a problem hiding this comment.
I think there's some mix of junit 4/5 annotations due to which AfterEach was not firing. Updated to use junit 4's annotation for clean up.
aho135
left a comment
There was a problem hiding this comment.
Thanks for the contribution @jaykanakiya! The changes look good to me. I'll let @clintropolis take a pass through as well. I think it would be fine to do the suggested refactoring in a follow up
| ); | ||
| } | ||
|
|
||
| private long verifyEncodedValues( |
There was a problem hiding this comment.
Ahh okay. I see we're truncating for MVD's with a single value. Would be good to add a test case for that also if that's the intended behavior
docs/configuration/index.md
Outdated
| |`druid.indexer.task.tmpStorageBytesPerTask`|Maximum number of bytes per task to be used to store temporary files on disk. This config is generally intended for internal usage. Attempts to set it are very likely to be overwritten by the TaskRunner that executes the task, so be sure of what you expect to happen before directly adjusting this configuration parameter. The config is documented here primarily to provide an understanding of what it means if/when someone sees that it has been set. A value of -1 disables this limit. |-1| | ||
| |`druid.indexer.task.allowHadoopTaskExecution`|Conditional dictating if the cluster allows `index_hadoop` tasks to be executed. `index_hadoop` is deprecated, and defaulting to false will force cluster operators to acknowledge the deprecation and consciously opt in to using index_hadoop with the understanding that it will be removed in the future.|false| | ||
| |`druid.indexer.server.maxChatRequests`|Maximum number of concurrent requests served by a task's chat handler. Set to 0 to disable limiting.|0| | ||
| |`druid.indexing.formats.maxStringLength`|Maximum number of characters to store per string dimension value. Longer values are truncated during ingestion. Set to 0 to disable. Can be overridden per-dimension using `maxStringLength` in the [dimension object](../ingestion/ingestion-spec.md#dimension-objects).|0 (no truncation)| |
There was a problem hiding this comment.
It would be good to mention that truncation does not apply for MVD's
There was a problem hiding this comment.
I added the test and doc update in the next commit.
Summary
Adds a configurable maximum string length for string dimension columns. Strings exceeding the limit are truncated during ingestion.
druid.indexing.formats.maxStringLengthmaxStringLengthfield in the dimension specRelease note
Added a new maxStringLength configuration for string dimensions that truncates values exceeding the specified length during ingestion. Can be set globally via druid.indexing.formats.maxStringLength or per-dimension in the ingestion spec.
Key changed/added classes in this PR
This PR has: